Add --force-refresh flag to CLI token source#1604
Add --force-refresh flag to CLI token source#1604mihaimitrea-db wants to merge 1 commit intomainfrom
Conversation
f626fed to
67f79d8
Compare
67f79d8 to
db4df21
Compare
Range-diff: main (67f79d8 -> db4df21)
Reproduce locally: |
db4df21 to
97a1007
Compare
Range-diff: main (db4df21 -> 97a1007)
Reproduce locally: |
97a1007 to
69a7c95
Compare
Range-diff: main (97a1007 -> 69a7c95)
Reproduce locally: |
config/cli_token_source.go
Outdated
| // forceCmd uses --profile with --force-refresh to bypass the CLI's token cache. | ||
| // Nil when cfg.Profile is empty (--force-refresh requires --profile support). | ||
| forceCmd []string | ||
|
|
||
| // hostCmd is a fallback command using --host, used when the primary --profile | ||
| // command fails because the CLI is too old to support --profile. | ||
| // profileCmd uses --profile for token lookup. Nil when cfg.Profile is empty. | ||
| profileCmd []string | ||
|
|
||
| // hostCmd uses --host as a fallback for CLIs that predate --profile support. | ||
| // Nil when cfg.Host is empty. | ||
| hostCmd []string |
There was a problem hiding this comment.
Can you add maybe versions of the CLI in which each flag was added?
| if cfg.Host != "" { | ||
| hostCmd = buildHostCommand(cliPath, cfg) | ||
| } |
There was a problem hiding this comment.
What about force refresh with the host set?
config/cli_token_source_test.go
Outdated
| @@ -327,21 +412,27 @@ func TestCliTokenSource_Token_NoFallbackOnRealError(t *testing.T) { | |||
|
|
|||
| tempDir := t.TempDir() | |||
|
|
|||
| // Primary script fails with a real auth error (not unknown flag). | |||
| // forceCmd fails with a real auth error (not unknown flag). | |||
| forceScript := filepath.Join(tempDir, "force_cli.sh") | |||
| if err := os.WriteFile(forceScript, []byte("#!/bin/sh\necho 'cache: databricks OAuth is not configured for this host' >&2\nexit 1"), 0755); err != nil { | |||
| t.Fatalf("failed to create force script: %v", err) | |||
| } | |||
|
|
|||
| // profileCmd and hostCmd should not be called. | |||
| profileScript := filepath.Join(tempDir, "profile_cli.sh") | |||
| if err := os.WriteFile(profileScript, []byte("#!/bin/sh\necho 'cache: databricks OAuth is not configured for this host' >&2\nexit 1"), 0755); err != nil { | |||
| if err := os.WriteFile(profileScript, []byte("#!/bin/sh\necho 'should not reach here' >&2\nexit 1"), 0755); err != nil { | |||
| t.Fatalf("failed to create profile script: %v", err) | |||
| } | |||
|
|
|||
| // Fallback script would succeed, but should not be called. | |||
| hostScript := filepath.Join(tempDir, "host_cli.sh") | |||
| if err := os.WriteFile(hostScript, []byte("#!/bin/sh\necho 'should not reach here' >&2\nexit 1"), 0755); err != nil { | |||
| t.Fatalf("failed to create host script: %v", err) | |||
| } | |||
|
|
|||
| ts := &CliTokenSource{ | |||
| cmd: []string{profileScript}, | |||
| hostCmd: []string{hostScript}, | |||
| forceCmd: []string{forceScript}, | |||
| profileCmd: []string{profileScript}, | |||
| hostCmd: []string{hostScript}, | |||
| } | |||
| _, err := ts.Token(context.Background()) | |||
| if err == nil { | |||
| @@ -351,3 +442,33 @@ func TestCliTokenSource_Token_NoFallbackOnRealError(t *testing.T) { | |||
| t.Errorf("Token() error = %v, want error containing original auth failure", err) | |||
| } | |||
| } | |||
|
|
|||
| func TestCliTokenSource_Token_NilHostCmdReturnsError(t *testing.T) { | |||
| if runtime.GOOS == "windows" { | |||
| t.Skip("Skipping shell script test on Windows") | |||
| } | |||
|
|
|||
| tempDir := t.TempDir() | |||
There was a problem hiding this comment.
How about righting them as table tests? Because they seem to be testing the same flow.
NEXT_CHANGELOG.md
Outdated
|
|
||
| ### New Features and Improvements | ||
|
|
||
| * Pass `--force-refresh` to Databricks CLI `auth token` command to bypass the CLI's internal token cache. |
There was a problem hiding this comment.
This seems to be an internal change, right? Because IIUC it was not broken before.
config/cli_token_source.go
Outdated
| if flag == "" { | ||
| return strings.Contains(err.Error(), "unknown flag:") | ||
| } |
There was a problem hiding this comment.
Why do you need this condition?
Pass --force-refresh to the Databricks CLI auth token command to bypass the CLI's internal token cache. The SDK manages its own token caching, so the CLI serving stale tokens from its cache causes unnecessary refresh failures. Commands are now tried in order: forceCmd (--profile + --force-refresh), profileCmd (--profile), hostCmd (--host), falling back progressively for older CLI versions that don't support newer flags. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
69a7c95 to
a314000
Compare
Range-diff: main (69a7c95 -> a314000)
Reproduce locally: |
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
🥞 Stacked PR
Use this link to review incremental changes.
Summary
Pass
--force-refreshto the Databricks CLIauth tokencommand so the SDK always receives a fresh token instead of a potentially stale one from the CLI's internal cache.Why
The SDK manages its own token caching via
CachedTokenSource. When the SDK decides it needs a new token and shells out todatabricks auth token, the CLI may return a cached token that is about to expire (or has already expired from the SDK's perspective). This creates unnecessary refresh failures and retry loops.The CLI recently added a
--force-refreshflag (databricks/cli#4767) that bypasses its internal cache. By using this flag, the SDK is guaranteed a freshly minted token every time it asks for one, eliminating the stale-token problem.What changed
Interface changes
None.
CliTokenSourceis not part of the public API surface.Behavioral changes
--force-refreshwhen invokingdatabricks auth token. When a profile is configured,--force-refreshis appended to the--profilecommand. When only a host is configured,--force-refreshis appended to the--hostcommand instead.--force-refresh, the SDK falls back to the plain--profileor--hostcommand (and then to--hostif--profileis also unsupported)."Databricks CLI does not support --force-refresh flag. The CLI's token cache may provide stale tokens. Please upgrade your CLI to the latest version.""Databricks CLI does not support --profile flag. Falling back to --host. Please upgrade your CLI to the latest version."Internal changes
CliTokenSourcenow holds three command variants:forceCmd(--force-refresh),profileCmd(--profile), andhostCmd(--host). Any of these can be nil depending on the config.buildCliCommandsreturns all three commands.forceCmdis based onprofileCmdwhen a profile is set, or onhostCmdwhen only a host is configured — so host-only setups also benefit from cache bypass.Token()tries commands in order (forceCmd→profileCmd→hostCmd), falling through on"unknown flag:"errors and logging a warning at each step.isUnknownFlagErroris now parameterized: pass a specific flag name to match against the CLI's error output.execCliCommanduses%qfor stderr formatting so multi-line CLI output (usage text on unknown-flag errors) stays on one line in logs.How is this tested?
Unit tests in
config/cli_token_source_test.go:TestBuildCliCommands— verifies all three commands are built correctly for each config combination (host-only, account host, profile+host, profile-only), including force-refresh based on host for host-only configs.TestCliTokenSource_Token_Fallback— table-driven test covering:forceCmdsucceeds (happy path)forceCmdfails → fallback toprofileCmdforceCmdfails → fallback tohostCmd(no profile configured)profileCmdfails → fallback tohostCmdforceCmd→profileCmd→hostCmd